Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

reverseproxy: Set the headers in the replacer before handle_response #4165

Merged
merged 1 commit into from
May 12, 2021
Merged

reverseproxy: Set the headers in the replacer before handle_response #4165

merged 1 commit into from
May 12, 2021

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented May 12, 2021 •
edited
Loading

Turns out this was an oversight, we assumed we could use {http.response.header.*} but that doesn't work because those are grabbed from the response writer, and we haven't copied any headers into the response writer yet.

So the fix is to set all the response headers into the replacer at a new namespace before running the handlers.

This adds the {http.reverse_proxy.header.*} replacer.

See https://caddy.community/t/empty-http-response-header-x-accel-redirect/12447

@francislavoie
reverseproxy: Set the headers in the replacer before handle_response
b30b979 
Turns out this was an oversight, we assumed we could use `{http.response.header.*}` but that doesn't work because those are grabbed from the response writer, and we haven't copied any headers into the response writer yet.

So the fix is to set all the response headers into the replacer at a new namespace before running the handlers.

This adds the `{http.reverse_proxy.header.*}` replacer.

See https://caddy.community/t/empty-http-response-header-x-accel-redirect/12447
@francislavoie francislavoie added the bug 🐞 Something isn't working label May 12, 2021
@francislavoie francislavoie added this to the v2.4.1 milestone May 12, 2021
@francislavoie francislavoie requested a review from mholt May 12, 2021 04:27
@francislavoie francislavoie changed the title ci: Run CI on PRs targeting minor version branches (#4164) reverseproxy: Set the headers in the replacer before handle_response May 12, 2021
@LecrisUT LecrisUT mentioned this pull request May 12, 2021
Closed
mholt
mholt approved these changes May 12, 2021
View reviewed changes
Copy link
Member

@mholt mholt left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, interesting situation... I guess this'll work, assuming we never need to differentiate request headers and response headers in the future.

@mholt mholt merged commit aef8d4d into 2.4 May 12, 2021
@mholt mholt deleted the handle-response-headers branch May 12, 2021 20:19
@francislavoie
Copy link
Member Author

francislavoie commented May 12, 2021 •
edited
Loading

I think we'd go with http.reverse_proxy.request_header.* if we ever did, which matches the request_header handler.

@francislavoie francislavoie mentioned this pull request May 13, 2021
Merged
@corny
Copy link

corny commented May 18, 2021 •
edited
Loading

I have another related issue: If the upstream server returns a Content-Disposition header, it won't be copied into the response. I need to add this line to the handle_response block:

header Content-Disposition "{http.reverse_proxy.header.Content-Disposition}"

Update: Same applies to the Content-Type header.

@francislavoie
Copy link
Member Author

That's normal. No headers are copied by default in handle_response, you need to do it yourself for anything that's relevant. It just gives you a clean slate to re-handle the request based on the response from the upstream.

@corny
Copy link

corny commented May 18, 2021

I see that Set-Cookie, X-Request-Id and a couple of other headers are copied. That's a little bit confusing, isn't it?

@francislavoie
Copy link
Member Author

francislavoie commented May 18, 2021 •
edited
Loading

Where do you see that @corny? The code doesn't copy any headers.

caddy/modules/caddyhttp/reverseproxy/reverseproxy.go

Lines 613 to 652 in b82db99

for i, rh := range h.HandleResponse {
if rh.Match != nil && !rh.Match.Match(res.StatusCode, res.Header) {
continue
}
// if configured to only change the status code, do that then continue regular proxy response
if statusCodeStr := rh.StatusCode.String(); statusCodeStr != "" {
statusCode, err := strconv.Atoi(repl.ReplaceAll(statusCodeStr, ""))
if err != nil {
return caddyhttp.Error(http.StatusInternalServerError, err)
}
if statusCode != 0 {
res.StatusCode = statusCode
}
break
}
// otherwise, if there are any routes configured, execute those as the
// actual response instead of what we got from the proxy backend
if len(rh.Routes) == 0 {
continue
}
res.Body.Close()
// set up the replacer so that parts of the original response can be
// used for routing decisions
for field, value := range res.Header {
repl.Set("http.reverse_proxy.header."+field, strings.Join(value, ","))
}
repl.Set("http.reverse_proxy.status_code", res.StatusCode)
repl.Set("http.reverse_proxy.status_text", res.Status)
h.logger.Debug("handling response", zap.Int("handler", i))
if routeErr := rh.Routes.Compile(next).ServeHTTP(rw, req); routeErr != nil {
// wrap error in roundtripSucceeded so caller knows that
// the roundtrip was successful and to not retry
return roundtripSucceeded{routeErr}
}
}

What does your Caddyfile look like? How did you build Caddy?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mholt mholt mholt approved these changes

Assignees
No one assigned
Labels
bug 🐞 Something isn't working
Projects
None yet
Milestone
v2.4.1
Development

Successfully merging this pull request may close these issues.
3 participants
@francislavoie @corny @mholt